feat: reintroduce nix flake #1683
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This exposes the `cargo pgrx` binary as a flake package called `cargo-pgrx`. It can be built using `nix build .#cargo-pgrx`. The systems specified in this flake were chosen from the project's README of tested systems. The flake inputs were chosen to minimize the maintenance burden. Since the build runs tests in order to succeed, but the tests are performed at build time for nix, the `test_parse_managed_postmasters` test has been skipped. Since pgrx already has a test suite that runs outside of nix, this should be a good compromise to allow the build to work, but also keep the majority of the tests to ensure that nix users are aware when they are broken. In addition to this, the `PGRX_HOME` variable must be set for tests, so it is assigned to a tmpdir before running the tests.
This script can be ran to update the rust-toolchain that is used by nix builds. Since `fenix` is a flake input end-users are able to override it themselves to whichever version they please, however it is useful to have the provided version up to date, particularly if the project ends up having an MSRV. We could likely change this script to be one that updates all flake inputs, however I find it useful to be able to update the rust toolchain separately from the other dependencies and have chosen to provide a script soley for that. This matches the existing `rustup.sh` that is used to update the rustup toolchain version. If you wish to make this file a generic `flake.lock` updater, remove `fenix` from the call to `nix flake update` and it will update all inputs.
This function can be used by external nix flakes to build an extension using pgrx. The caller must provide the postgres package, the rust toolchain, extension source code, and flake system that they are building for. Most of this configuration has been converted from the previous nix flake exposed from pgrx. I've made every attempt to trim away parts that don't serve a purpose, but there's a chance that this could be simplified slightly by someone who has more knowledge of pgrx than I do.
|
I've opened this as a draft PR so it can gather attention while I try to see if I can somehow test on |
justinrubek
commented
Apr 28, 2024
| pname = "${name}-pg${postgresMajor}"; | ||
| nativeBuildInputs = [ | ||
| pkgs.pkg-config | ||
| pkgs.rustPlatform.bindgenHook |
There was a problem hiding this comment.
bindgenHook is a handy package I found that sets BINDGEN_EXTRA_CLANG_ARGS, which was previously being done manually in the build step. See https://github.com/symphorien/nixpkgs/blob/master/pkgs/build-support/rust/hooks/rust-bindgen-hook.sh
Comment on lines
+98
to
+101
| PGRX_PG_SYS_SKIP_BINDING_REWRITE = "1"; | ||
| CARGO = "${rustToolchain}/bin/cargo"; | ||
| CARGO_BUILD_INCREMENTAL = "false"; | ||
| RUST_BACKTRACE = "full"; |
There was a problem hiding this comment.
These get set as environment variables for the build
|
Thanks @justinrubek. If you will, give us a bit of time to digest this. A quick comment is I think we'd want to see some kind of documentation in our fledgling "book". Probably from two perspectives. One being general Nix users, and the other being me and @workingjubilee and @thomcc and @NotGyro so we know how to update the things that might need updating in the future. Additionally, I'm sure we'll require a separate CI job that tests a nix build of just the simple |
|
Absolutely take your time, I'm in no hurry. I'm very responsive to GitHub notifications, and I'll do my best to address everything. I can get started on those pieces and anything that is further requested as it comes in. |
|
This pr in nixpkgs may also be relevant for seeing some pgrx extensions successfully build on |
|
@justinrubek You asked me on the pgrx discord to post this on this PR: I saw you had an example repo (pgrx-nix-example), and gave it a spin. I ran Happy to provide additional debug info if you'd like. |
|
@justinrubek if I build on I think you will actually need this However, even with that patch, which stops the nix build from erroring, I am now running into the same issue I see on our supabase extenions when building on |
That example doesn't provide a devShell so Sidenote: I've not been active on the discord, so it wasn't me that sent you here. Nevertheless, I will provide assistance as best as I can. |
It's hard for me to test darwin builds, but I believe that the optional darwin portion needs |
|
@justinrubek yaeh sure, I have an aarch64-darwin machine right here, count me in on helping you with this I'll try that let you know and keep trying to build this for you too |
|
@justinrubek that did it for builds successfully with those changes on aarch64-darwin |
|
tested and confirmed project builds on |
Co-authored-by: Sam Rose <[email protected]>
@justinrubek OK, thanks for the explanation. Yes, it worked perfectly. Sorry for the distraction! |
|
@justinrubek @workingjubilee I'd love to try and help move this forward if possible. Let me know if there are things I can try to take on beyond testing on aarch64-darwin |
The main thing left would be to address the request from this comment #1683 (comment) We need to provide enough information for the maintainers to feel comfortable having this code without it decaying. Things like updating the flake inputs and a test case with the example project. I've been particularly busy since opening this so I haven't started on it. I'm not sure where the mentioned book is. Those things would be good to take on. |
|
@justinrubek thanks. I am also trying to specifically build an |
|
@samrose Just to be clear, that example project isn't going to be integrated into this project. They would want the extension generated using |
|
@justinrubek makes sense. I would need to make this PR work, plus make your example work using the builder I may gain some insights by trying to use the builder on different systems that could help this PR I believe. But I'll also help here with #1683 (comment) |
|
Coming back to this now. I believe the hanging tasks here were #1683 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
overview
This reintroduces the nix code after it was removed in #1682. It has been refactored to live in a single file and many parts of it have been removed because they weren't necessary. I will attempt to provide an overview of everything that is taking place in this configuration and why I've chosen to write things the way I have. Additionally, my commit messages have context if you expand them.
The previous flake did the following things:
cargo-pgrxbinaryI've chosen to focus on the first two options in the above list. There is no need to verify formatting using nix since this is done through GitHub Actions. I could feasibly make an example, but I'll only do that once I'm sure that this PR can be accepted (and if it is requested). For now, as part of my testing, I've taken the arrays example from the
pgrx-examplesdirectory and made a separate repo showcasing how to use this new flake https://github.com/justinrubek/pgrx-nix-exampleflake inputs
Here is what each input does and why we're using them:
cargo-pgrxand also to call pgrx when building an extensionperSystemfunction inflake.nix.gitdirectory from being copied into buildsFor the most part we should be fine to leave these dependencies locked to their current version in
flake.lock. However, it isn't a bad idea to try to keep them up to date. I've provided a shell script that will update thefenixinput to retrieve newer rust toolchains, although I could see that it may better suit the project to have the script update every input so I will change it if asked.flake outputs
This flake exposes two outputs:
packages.${system}.cargo-pgrxandlib.buildPgrxExtensioncargo-pgrx
This is the pgrx binary, built using the source code from this repo. It's build is a pretty standard cargo build, but there are a few extra things happening.
As part of the build step the tests are ran. However, pgrx's tests appear to have two requirements that won't be met by default:
PGRX_HOMEenvironment variable must be settest_parse_managed_postmasters,cargo pgrx initmust have been ranFor 1. the solution is easy: the
preCheckblock is ran which sets the file to a temporary directory. However, 2. is not as simple because we don't have the pgrx binary available yet due to this being the build step for it. The compromise is to skip any tests that require this to be ran. The rest of the tests are still ran, so we can be fairly sure the nix build isn't broken. And since the pgrx test suite is not being ran through nix any issues with this test will be detected anyway.This should require minimal maintenance as building a cargo package is fairly trivial. The only exceptions are:
buildInputsto contain the package corresponding to the new dependency. For a list of packages, see https://search.nixos.org/packages?channel=unstablebuildPgrxExtension
This is the more complicated piece, but it is also the larger value-add for nix users. I've taken most of the logic from the previous configuration, distilling it down by removing extra code and simplifying the rest. I believe I've gotten it nearly as simple as it can be, but there may be some further room for improvement.
The function is responsible for setting up postgres in a way that stops
pgrxfrom attempting to download it. This is because flake builds are executed in a sandbox without network access. Consuming flakes must provide the postgres package they wish to use in order for this to work. The function will determine the major version to set up the proper feature flags as well as put postgres in a subdirectory ofPGRX_HOMEaccording to its version (this is thepreBuildAndTestblock).Apart from that, there is some file cleanup that takes place in the
preFixupandpostInstallsteps, primarily to ensure that only the minimum things required end up in the final build (if I understanding right, the--out-dirflag frompgrxis placing things there that are not desired in the final build, so I chose to delete them).Testing performed
I have tested this all using
x86_64-linux. I am unable to testaarch64-linux, although I may be able to tryaarch64-darwin. I suspect that it will work onaarch64-linuxanyway.I've verified that I can build extensions and load them into postgres. See justinrubek/pgrx-nix-example#1 (comment). I've also back-ported this change to pgrx v0.11 and tested using pgx_ulid.
further work
I don't have a wide variety of extensions to test against. I can imagine the need for some tweaks depending on any issues encountered by people using this flake, but I'm not sure what those would be at this time. I am willing and able to look into this should any come up, so please route these to me so I can help.
Beyond that, I have a few small changes that may be good:
buildPgrxExtensionswhy this is important for nix users
There is a
cargo-pgrxbinary andbuildPgrxExtensionfunction in nixpkgs, but that is not sufficient due to thepgrxbinary version needing to match the cargo dependency. If a consumer wishes to use a newer version of pgrx, they will have to wait for it to make it into nixpkgs. On the other side of things, if they wish to remain on an older version of pgrx they may have to pin an old nixpkgs. By keeping the flake in this repo consumers are able to easily use the latest version of pgrx without any difficulty. Here's an example of the supabase folks dealing with a glibc issue where they pray updatingcargo-pgrxwill resolve the issue supabase/nix-postgres#28 (comment), which is precisely the kind of thing I'd like to avoid.